Harden SDK hook context and capability diagnostics - #105

@codex review
/gemini review
@gemini-code-assist review

Please review the SDK hook context and manifest capability guardrail changes, especially compatibility of optional onWrite payload fields, package doctor diagnostics, and package-first dogfood gating.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@codex review
/gemini review
@gemini-code-assist review
@coderabbitai full review

Follow-up commit a6088f1 addresses the actionable CodeRabbit finding by only swallowing ENOENT when probing optional first-party extension source files. Please re-review the updated PR for any remaining SDK hook context, manifest capability, package doctor, and changelog/dogfood issues.

#!/bin/bash
echo "=== extension-types.ts ==="
cat src/core/extensions/extension-types.ts

echo ""
echo "=== loader.ts ==="
cat src/core/extensions/loader.ts

echo ""
echo "=== extension/doctor.ts ==="
cat src/cli/commands/extension/doctor.ts

echo ""
echo "=== extension.ts (relevant section) ==="
cat src/cli/commands/extension.ts

echo ""
echo "=== item-store.ts (relevant sections) ==="
cat src/core/store/item-store.ts

echo ""
echo "=== package-import-adapters.ts ==="
cat src/sdk/package-import-adapters.ts

echo ""
echo "=== create.ts ==="
cat src/cli/commands/create.ts

echo ""
echo "=== restore.ts ==="
cat src/cli/commands/restore.ts

echo ""
echo "=== dogfood-package-first.mjs ==="
cat scripts/dogfood-package-first.mjs

echo ""
echo "=== package-manifest.spec.ts ==="
cat tests/unit/package-manifest.spec.ts

echo ""
echo "=== item-store.spec.ts ==="
cat tests/unit/item-store.spec.ts

echo ""
echo "=== extension-command.spec.ts ==="
cat tests/unit/extension-command.spec.ts

#!/bin/bash
# Fetch the PR diff and file list via GitHub API
gh pr view 105 --repo unbraind/pm-cli --json files,title,body,commits | jq '.'

@unbraind: Thanks for the update — the ENOENT-only swallowing fix in a6088f1 is the correct approach. Here is the re-review across all four focus areas plus the commander bump.

✅ What looks good

SDK hook context (OnWriteHookContext)

All five new fields (item_id, item_type, before, after, changed_fields) are optional, so existing hook implementations continue to compile and run without changes — full backward compatibility.

The ordering fix in src/cli/commands/create.ts — computing changedFields before calling runActiveOnWriteHooks instead of after — is correct and important; hooks now receive the data they need when they run.

In src/sdk/package-import-adapters.ts, sharing a single beforeDocument reference across both hook invocations is a clean deduplication.

Manifest capability guardrails

assertExtensionCapability in loader.ts now throws a structured createRegistrationValidationError with a full ExtensionActivationFailureTrace payload instead of a bare TypeError. That's a meaningful improvement for downstream diagnostics.

classifyDoctorActivationFailureWarnings correctly deduplicates and sorts warnings before emitting them, preventing duplicate extension_capability_missing codes for the same extension/capability pair.

Package doctor diagnostics

Running package doctor --detail deep --trace and deriving expectations from the returned details payload (rather than a static summary) is a solid upgrade that will catch real problems in CI.

Dogfood gating

The triple assertion in dogfood-package-first.mjs — zero activation failures, zero blocking failures, no extension_capability_missing codes — is a strong first-party guardrail.

⚠️ Remaining findings

1. Commander 15 — ESM-only and Node.js ≥ 22.12.0 requirement (medium)

Commander 15 is ESM-only; the deprecated commander/esm.mjs export was removed. This is expected to be seamless for ESM consumers, but some CommonJS consumers may hit issues with tooling requiring configuration for ESM-only dependencies.

Commander 15 also requires Node.js v22.12.0 or higher (for require(esm)).

Action needed: Confirm the project's declared engines.node lower bound in package.json is >=22.12.0 (or higher). If the project currently advertises support for older Node versions, the ^15.0.0 pin is a silent runtime breakage for those users. Also confirm no part of the test pipeline or CI matrix still runs on Node 20.

Additionally, Commander 15 changed default option value behavior: only a lone --no-* option now sets the default value to true — if any pm-cli command defines both a positive and a negative variant of the same flag, verify the default is still what you expect.

2. registration_index: -1 sentinel in ExtensionActivationFailureTrace (low)

loader.ts sets registration_index: -1 in the trace payload when assertExtensionCapability fires before any numbered registration entry. -1 is a valid array index sentinel in JavaScript, but nothing in ExtensionActivationFailureTrace documents that -1 means "pre-registration / capability gate" rather than "last element." Consumers parsing the trace field in doctor output may misinterpret it.

Suggestion: Either add a JSDoc comment to registration_index in extension-types.ts noting that -1 signals a pre-registration capability check failure, or add an explicit optional boolean field like pre_registration?: true to the trace type.

3. changed_fields for import operations undocumented (low)

src/sdk/package-import-adapters.ts now passes changed_fields to runActiveOnWriteHooks, but the AI summary does not state what value is used (e.g., ["imported"], an empty array, or something else). docs/SDK.md documents changed_fields for item writes but the import path is not called out.

Suggestion: Add a note to docs/SDK.md under the onWrite context section stating what changed_fields contains for op: "create" writes triggered by pm package import, parallel to the existing documentation for create, update, and delete.

4. package-manifest.spec.ts — coverage limited to index.ts / index.js only (low)

The new first-party capability guard test scans only index.ts and index.js for register* call patterns. Extensions that split their registration logic across multiple source files (e.g., a src/register-commands.ts helper) won't be covered, and the regex scan won't catch dynamically composed or re-exported registration calls.

This is a reasonable best-effort guard, but it should be annotated so future contributors don't rely on it as a complete check:

// NOTE: This is a heuristic text scan of index.ts / index.js only.
// It does not cover multi-file registrations or dynamic patterns.
// Treat failures as definitive but passes as best-effort only.

5. Synthetic changed_fields sentinels not reflected in docs (nit)

restore.ts uses changed_fields: ["restored"] and item-store.ts (delete path) uses changed_fields: ["deleted"]. These are synthetic sentinels, not actual document field names. docs/SDK.md should call these out explicitly so hook authors know to handle them as lifecycle signals rather than schema fields. Currently the doc describes changed_fields as the list of mutated fields, which is accurate for update operations but misleading for restore and delete.

Summary

Learnt from: CR
Repo: unbraind/pm-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-20T21:10:16.414Z
Learning: Use sandbox-safe commands for verification. For documentation-only work, run at least `pnpm build` and a focused link/content check. For broader work, run linked tests, coverage, validation, and release gates.

@codex review
/gemini review
@gemini-code-assist review
@coderabbitai full review

Follow-up commit 0077666 addresses the latest review pass: commander is reverted to 14.0.3 to preserve package.json Node >=20 and CI Node 20 compatibility, SDK docs now call out lifecycle changed_fields sentinels, registration_index=-1 is documented as a pre-registration trace sentinel, and the static manifest guard is annotated as heuristic. Please re-review for any remaining actionable issues.

#!/bin/bash
echo "=== package.json (commander dep) ==="
grep -n "commander" package.json

echo ""
echo "=== docs/SDK.md (onWrite section + changed_fields) ==="
cat docs/SDK.md

echo ""
echo "=== src/core/extensions/extension-types.ts ==="
cat src/core/extensions/extension-types.ts

echo ""
echo "=== tests/unit/package-manifest.spec.ts ==="
cat tests/unit/package-manifest.spec.ts

echo ""
echo "=== src/sdk/package-import-adapters.ts (changed_fields usage) ==="
grep -n "changed_fields\|import" src/sdk/package-import-adapters.ts | head -60

echo ""
echo "=== docs/EXTENSIONS.md (onWrite section) ==="
cat docs/EXTENSIONS.md

#!/bin/bash
# Use GitHub API to read files on the PR branch (head ref)
REPO="unbraind/pm-cli"
BRANCH=$(gh pr view 105 --repo "$REPO" --json headRefName --jq '.headRefName')
echo "Branch: $BRANCH"

for FILE in \
  "package.json" \
  "docs/SDK.md" \
  "src/core/extensions/extension-types.ts" \
  "tests/unit/package-manifest.spec.ts" \
  "src/sdk/package-import-adapters.ts"
do
  echo ""
  echo "====== $FILE ======"
  gh api "repos/$REPO/contents/$FILE?ref=$BRANCH" --jq '.content' | base64 -d 2>/dev/null || echo "(failed to decode)"
done

@unbraind: The sandbox was unavailable for both script attempts this pass, so I'm working from the PR description, commit message, AI summaries, and the previous review chain. Here is the re-review.

Re-review after commit 0077666

Previously flagged findings — claimed addressed

All five items were substantive and the described fixes are appropriate. No concerns about the fix approach for any of them.

One remaining low-priority flag (new)

src/sdk/package-import-adapters.ts — shared beforeDocument reference across two hook invocations

The AI summary notes that a single beforeDocument = emptyImportedDocument() is computed once and passed as before to both the item-file hook and the history-file hook. If any onWrite hook implementation mutates the before payload (e.g. context.before.status = "processed"), the second hook invocation will receive the already-mutated object rather than the original empty document.

Since OnWriteHookContext.before is typed as ItemDocument (not Readonly<ItemDocument>), there is no type-level protection against mutation. This is a correctness concern only if hooks mutate payload fields, which is an unusual pattern, but the shared reference means it could produce subtly wrong behavior.

Suggestion (low priority): Shallow-copy the reference at each call site, or freeze the object before passing:

before: Object.freeze({ ...beforeDocument }),

or accept it as-is and document in docs/SDK.md that hook implementations must treat before/after as read-only (similar to React's props contract). The latter is lower overhead and consistent with how SDKs typically frame payload immutability.

Everything else

All other areas — the structured capability error in loader.ts, classifyDoctorActivationFailureWarnings deduplication, dogfood triple assertions, OnWriteHookContext backward compatibility, the new unit tests for item-store and extension-command — remain solid from the previous pass and nothing in the AI summary for 0077666 touches those paths.

The PR is in good shape. The shared-reference issue above is the only new item, and it is low-priority / best addressed with a documentation note if not a code change.

Learnt from: CR
Repo: unbraind/pm-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-20T21:10:16.414Z
Learning: Use sandbox-safe commands for verification. For documentation-only work, run at least `pnpm build` and a focused link/content check. For broader work, run linked tests, coverage, validation, and release gates.

Learnt from: unbraind
Repo: unbraind/pm-cli PR: 0
File: :0-0
Timestamp: 2026-05-21T22:17:12.545Z
Learning: In `unbraind/pm-cli`, the semantic/hybrid search fallback-on-any-error behavior in `src/cli/commands/search.ts` is intentional. The product requirement is that `pm search` must never block an agent. Every degradation emits a machine-readable `search_<mode>_fallback:<reason>:using_keyword_mode` warning (reason ∈ `timeout`|`connection`|`error`) returned to the caller and captured by telemetry. Sentry was explicitly rejected for this path because it is lazily initialized only in error paths and would impose async init latency on the hot (successful) search path.

Learnt from: unbraind
Repo: unbraind/pm-cli PR: 0
File: :0-0
Timestamp: 2026-05-21T22:17:12.545Z
Learning: In the `unbraind/pm-cli` repository, files under `.agents/pm/` (with extensions `.toon` and `.jsonl`) are generated and managed exclusively by the `pm` CLI tool itself (via `pm create`, `pm close`, `pm update`, etc.). They are never manually edited. Committing these files alongside the work they track is the established convention in this repo (see PRs `#36`–#39). These files should NOT be flagged with "do not edit directly" or similar warnings in code review.